Fix type keyword completions #32474

sandersn · 2019-07-18T21:47:07Z

In functions, type keywords were omitted.
In All context, no keywords were omitted.

(1) fixes #28737
(2) removes 17 keywords that should not be suggested, even at the
toplevel of a typescript file:

private
protected
public
static
abstract
as
constructor
get
infer
is
namespace
require
set
type
from
global
of

I don't know whether we have a bug tracking this or not.

1. In functions, type keywords were omitted. 2. In All context, no keywords were omitted. (1) fixes #28737 (2) removes 17 keywords that should not be suggested, even at the toplevel of a typescript file: * private * protected * public * static * abstract * as * constructor * get * infer * is * namespace * require * set * type * from * global * of I don't know whether we have a bug tracking this or not.

sandersn · 2019-07-18T21:52:05Z

Unless somebody knows how to un-update a submodule, I'm going to leave prettier in there. It's a result of git add * inside ~/ts/tests/cases

sheetalkamat · 2019-07-18T22:15:40Z

tests/cases/fourslash/completionInFunctionLikeBody.ts

@@ -19,7 +19,7 @@ verify.completions(
        includes: ["async", "await"].map(
            name => ({ name, sortText: completion.SortText.GlobalsOrKeywords })
        ),
-        excludes: ["public", "private", "protected", "constructor", "readonly", "static", "abstract", "get", "set"],
+        excludes: ["public", "private", "protected", "constructor", "static", "abstract", "get", "set"],


Why is readonly ok in at position 1 and 2 ?

we allow now type keywords there and readonly is a type keyword with things like readonly T[]

sheetalkamat · 2019-07-18T22:17:12Z

src/harness/fourslash.ts

            "async",
            "await",
+            "boolean",


Why? I don't think we should be showing these here

I think you want to check if
let m = new Map<string/**/ is isPossiblyTypeArgumentPosition and add type keywords + function value keywords

As far as I can tell, there's no more precise context than "inside a function" for completions. I think if we show things like class, var, interface there then type keywords make sense too.

don't think so. class, var, interface can really be used in the function context to start local declarations? why would you want types?

filterGlobalCompletion has code that looks like it tries to do this, but it relies exclusively on isTypeOnlyCompletion. Is that a good place to add a check for isPossiblyTypeArgumentPosition, do you think?

Instead of changing FunctionLikeBodyKeywords

sandersn · 2019-07-18T22:47:57Z

OK, the PR is updated with my better understanding of KeywordCompletionFilters.TypeKeywords, which is now set when the context is possibly a type argument.

I'm going to see if the two variants of isType* in filterGlobalCompletion can be simplified to one now.

sandersn · 2019-07-18T22:54:04Z

Looks like No, because the distinction allows us to continue to suggest values when something might be a type argument (that is, when the brackets are unclosed), and to only suggest types when something is definitely a type argument (when the brackets are correctly matched).

andrewbranch · 2019-07-18T23:09:47Z

src/harness/fourslash.ts

            "bigint",
-            "of",


This is awesome. What was the change that actually made all these disappear from globals though?

sandersn · 2019-07-18T23:14:19Z

src/services/completions.ts

@@ -1182,7 +1182,7 @@ namespace ts.Completions {
        function filterGlobalCompletion(symbols: Symbol[]): void {
            const isTypeOnly = isTypeOnlyCompletion();
            const allowTypes = isTypeOnly || !isContextTokenValueLocation(contextToken) && isPossiblyTypeArgumentPosition(contextToken, sourceFile, typeChecker);
-            if (isTypeOnly) {
+            if (allowTypes) {
                keywordFilters = isTypeAssertion()
                    ? KeywordCompletionFilters.TypeAssertionKeywords
                    : KeywordCompletionFilters.TypeKeywords;


note that this means we no longer offer other keywords in completions; new, this, typeof are the useful ones I saw from scanning the list. Probably this and typeof should be added to the list typeKeywords. Suggesting new isn't very useful since it only saves 3 characters (or less).

For example,

return x < this.property;

will not suggest this any more.

Probably this and typeof should be added to the list typeKeywords.

Agreed. That seems objectively correct, since those are valid in a type position.

But why wouldn’t we show both type and non-type keywords here, given the uncertainty? Related questions:

Are we trying to avoid semantic work here? It seems possible to differentiate between an x that can’t possibly accept type parameters (so we’re definitely not in a type position) vs. an x that’s generic (so we may or may not be).

If we are trying to avoid semantic work, would it be reasonable to use whitespace as a heuristic? Map <string, number> seems extraordinarily uncommon, and x<3 fairly uncommon too.

I’m not sure whether these are practical or not, but I don’t think they’re necessary for this PR—seems like an improvement already. Just food for thought. (I do think adding this and typeof now is probably worth it, though.)

Hmm. Both the simple "show all" idea and "be smarter about possible type arguments" idea are good.

For the second, isPossiblyTypeArgumentPosition already calls getTypeAtLocation, so it's just a matter of querying the returned value.

I added this and typeof as types and it broke find all refs of global this horribly. I haven't looked at why, but it means that I'll probably keep them where they are for now.

Yep, isPossiblyTypeArgumentPosition already is smarter about possible type arguments. My tests just don't show it.

In any case, my original example, x < this.property, is wrong -- it does suggest this. It just doesn't when the identifier to the left of < is a generic signature.

So I think making this and typeof type keywords is best fixed in a separate PR, since it requires more knowledge of services than I have now, and only affects things that are genuinely type contexts. I filed a bug #32486 to track it.

Ah, that sounds better. I was a little concerned that you’d be missing those any time you were in the RHS of a less-than arithmetic comparison. 👍

sandersn · 2019-07-18T23:24:42Z

src/services/completions.ts

@@ -2060,16 +2060,18 @@ namespace ts.Completions {
                case KeywordCompletionFilters.None:
                    return false;
                case KeywordCompletionFilters.All:
-                    return kind === SyntaxKind.AsyncKeyword || SyntaxKind.AwaitKeyword || !isContextualKeyword(kind) && !isClassMemberCompletionKeyword(kind) || kind === SyntaxKind.DeclareKeyword || kind === SyntaxKind.ModuleKeyword


@andrewbranch Here! SyntaxKind.AwaitKeyword isn't compared ===kind and since it's truthy it caused .All to let all keywords through.

DanielRosenwasser · 2019-07-19T00:55:05Z

Does this fix #27955? (it's okay if it doesn't, but just close it if it does)

sandersn · 2019-07-19T16:45:41Z

@DanielRosenwasser pretty sure it can't possibly, because implements isn't one of the keywords that are no longer suggested.

Also, there are some JS-specific checks that happen. They might avoid this fix entirely; I haven't tested that.

sheetalkamat · 2019-07-19T20:00:01Z

src/services/completions.ts

@@ -1182,7 +1182,7 @@ namespace ts.Completions {
        function filterGlobalCompletion(symbols: Symbol[]): void {
            const isTypeOnly = isTypeOnlyCompletion();
            const allowTypes = isTypeOnly || !isContextTokenValueLocation(contextToken) && isPossiblyTypeArgumentPosition(contextToken, sourceFile, typeChecker);
-            if (isTypeOnly) {
+            if (allowTypes) {


I don't think this is correct change. KeywordFilters = KeywordCompletionFilters.TypeKeywords would only offer type keywords? but that's not what you want here. I think you want isTypeOnly => KeywordCompletionFilters.TypeKeywords, otherwise if you have allowTypes then you want global + type keywords and rest of the time only global keywords ?

After some discussion, we decided that isPossiblyTypeArgumentPosition is smart enough to rely on only providing types when it's true.

This simplifies code that relies on isPossiblyTypeArgumentPosition, as well as some tests. We no longer fall back to types+values, instead suggesting one or the other. isPossiblyTypeArgumentPosition should have a very low false positive rate since it relies on types, which means that people could see values-only completions a little more than before. The most common failure case I can think of is when a variable that should have a generic signature type actually has type any. I think this is acceptable.

Because isPossiblyTypeArgumentPosition doesn't give false positives now that it uses type information.

sandersn requested review from andrewbranch and orta July 18, 2019 21:47

sheetalkamat reviewed Jul 18, 2019

View reviewed changes

Change keyword filter in filterGlobalCompletion

d6a9432

Instead of changing FunctionLikeBodyKeywords

andrewbranch approved these changes Jul 18, 2019

View reviewed changes

sandersn commented Jul 18, 2019

View reviewed changes

Add more tests cases

ca8c42e

sheetalkamat reviewed Jul 19, 2019

View reviewed changes

Make type-only completions after < more common

d8972c1

Because isPossiblyTypeArgumentPosition doesn't give false positives now that it uses type information.

sheetalkamat approved these changes Jul 19, 2019

View reviewed changes

sandersn merged commit e543d8b into master Jul 19, 2019

sandersn deleted the missing-primitive-completions-in-functions branch July 19, 2019 22:22

uniqueiniquity mentioned this pull request Sep 26, 2019

type and namespace no longer appear as global keyword completion #33615

Closed

alexkayabula mentioned this pull request Oct 6, 2019

Add missing global keyword completion #33835

Closed

mjbvz mentioned this pull request Jan 28, 2020

typescript specific keywords returns in js files #27955

Closed

fabioemoutinho mentioned this pull request Aug 13, 2021

Missing autocomplete on the "abstract" keyword #45260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type keyword completions #32474

Fix type keyword completions #32474

sandersn commented Jul 18, 2019

sandersn commented Jul 18, 2019

sheetalkamat Jul 18, 2019

sandersn Jul 18, 2019

sheetalkamat Jul 18, 2019

sheetalkamat Jul 18, 2019

sandersn Jul 18, 2019

sheetalkamat Jul 18, 2019

sandersn Jul 18, 2019

sandersn commented Jul 18, 2019

sandersn commented Jul 18, 2019

andrewbranch Jul 18, 2019

sandersn Jul 18, 2019

andrewbranch Jul 18, 2019

sandersn Jul 19, 2019

sandersn Jul 19, 2019

sandersn Jul 19, 2019

andrewbranch Jul 19, 2019

sandersn Jul 18, 2019

DanielRosenwasser commented Jul 19, 2019 •

edited

Loading

sandersn commented Jul 19, 2019

sheetalkamat Jul 19, 2019

sandersn Jul 19, 2019

Fix type keyword completions #32474

Fix type keyword completions #32474

Conversation

sandersn commented Jul 18, 2019

sandersn commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Jul 18, 2019

sandersn commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Jul 19, 2019 • edited Loading

sandersn commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Jul 19, 2019 •

edited

Loading